fix(@angular/ssr): normalize patched host headers#33126
fix(@angular/ssr): normalize patched host headers#33126Hexix23 wants to merge 1 commit intoangular:21.1.xfrom
Conversation
Return the same first host-header token that is validated so application code cannot receive unvalidated forwarded-host suffixes.
Hexix23
force-pushed
the
fix-ssr-normalize-patched-host-headers
branch
from
12a0f4b to
1c411bd
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security in Angular SSR by proactively normalizing host headers during the request cloning process. It ensures that headers like 'host' and 'x-forwarded-host' are set to their first validated token before the request is processed. New unit tests have been added to verify this behavior during direct header access and iteration. A review comment suggests also patching the set() and append() methods of the headers object to prevent subsequent modifications from bypassing the normalization logic.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/angular/ssr/src/utils/validation.ts (111-117)
The normalization of host headers is performed only once during the initialization of the cloned request. If application code or middleware subsequently modifies these headers using headers.set() or headers.append(), the normalization will be lost, although the patched get() method will still trigger validation on the first token of the new value. While this addresses the primary SSRF risk, consider if set() and append() should also be patched to maintain normalization throughout the request lifecycle for these specific headers.
|
Closing this PR. The vulnerable code path targeted here no longer exists in the current 21.2.x branch, where it has been replaced by the trusted proxy header flow. This PR was effectively a 21.1.x backport/hardening change, and I am withdrawing it rather than pursuing that backport. |
1 participant
This fix addresses an SSRF / host-confusion hardening issue in the
@angular/ssrcloneRequestAndPatchHeaders()code path on the 21.1.x release branch. The samecode path was present in earlier 21.2.x builds before it was replaced by the newer
trusted proxy header flow.
The affected code
cloneRequestAndPatchHeaders()validated only the first comma-separated tokenof
Host/X-Forwarded-Host, but returned the original raw header value toapplication code through the patched
Headersaccessors.This PR follows Google OSS VRP guidance to work with the Angular maintainers via
a public GitHub PR for this issue.
Changes:
HostandX-Forwarded-Hostheader values to the firstcomma-separated token before application code can read them
get(),entries(),values(),forEach(), andfor...ofreadsThis aligns the values returned to application code with the value Angular already
uses for validation and internal URL construction.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
cloneRequestAndPatchHeaders()patchesHeaders.get(),Headers.entries(),Headers.values(),Headers.forEach(), and[Symbol.iterator]so host headersare validated when application code reads them. The validation path uses the first
comma-separated header token. However, the patched accessors return the raw header
string.
For example, with
allowedHosts = new Set(['example.com']),X-Forwarded-Host: example.com,@127.0.0.1:8765validates asexample.com, butapplication code reading
request.headers.get('x-forwarded-host')receives theraw value
example.com,@127.0.0.1:8765.What is the new behavior?
The cloned request normalizes
HostandX-Forwarded-Hostto the same firstcomma-separated token that Angular validates. Application code therefore receives
example.comin the example above, rather than the unvalidated suffix.Other headers are unchanged; e.g.
Accept: text/html, application/jsonstillreturns the full value.
Does this PR introduce a breaking change?
Other information
Validated locally with:
bazelisk test //packages/angular/ssr/test:testBazel reported the target as flaky because the first attempt hit unrelated existing
app_spec.tsfailures, then retried and completed with the test target passing.